-
Notifications
You must be signed in to change notification settings - Fork 27.4k
refactor(ngAnimateSwap): remove unused dependency and add tests #16565
Conversation
@@ -132,6 +132,51 @@ describe('ngAnimateSwap', function() { | |||
expect(two).toBeTruthy(); | |||
})); | |||
|
|||
it('should create a new (non-isolate) scope for each inserted clone', inject(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this test proves that the new scopes are non-isolate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't an isolate scope be directly derived from the rootScope and make this fail: expect(scopeOne.$parent).toBe(parentScope)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - an isolate scope still has a parent but it doesn't inherit from its parent. So
$parent.foo = 'bar';
$isoChild = $parent.$new(true);
assert($isoChild.foo !== 'bar');
assert($isoChild.$parent.foo === 'bar');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops 😁
test/ngAnimate/ngAnimateSwapSpec.js
Outdated
expect(scopeTwo.$$destroyed).toBe(true); | ||
})); | ||
|
||
it('should destroy the previous scope before when swapping elements', inject(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... before ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM a couple of minor points
LGTM, but the |
…creating/removing scopes
21f5b6c
to
431dc5e
Compare
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactor.
Tests.
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Fix/Feature: Docs have been added/updated